Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

chore(ssa refactor): Implement function inlining #1293

Merged
merged 7 commits into from
May 9, 2023
Merged

chore(ssa refactor): Implement function inlining #1293

merged 7 commits into from
May 9, 2023

Conversation

jfecher
Copy link
Contributor

@jfecher jfecher commented May 4, 2023

Related issue(s)

Resolves #

Description

Summary of changes

Implements function inlining for the ssa refactoring. This pass will attempt to inline all functions into (a copy of) main. If it fails to inline all functions because a function in a function call is still unknown, it will keep each function in the Ssa struct and set the new main function to the partially-inlined version. If all functions are successfully inlined, the fully-inlined main function is the new main function and all other functions are removed.

Dependency additions / changes

Test additions / changes

Checklist

  • I have tested the changes locally.
  • I have formatted the changes with Prettier and/or cargo fmt with default settings.
  • I have linked this PR to the issue(s) that it resolves.
  • I have reviewed the changes on GitHub, line by line.
  • I have ensured all changes are covered in the description.

Documentation needs

  • This PR requires documentation updates when merged.

Additional context

@jfecher jfecher requested review from kevaundray and joss-aztec May 4, 2023 18:04
@jfecher
Copy link
Contributor Author

jfecher commented May 4, 2023

Open questions:

  1. Are the comments clear enough that you can more easily tell what is going on?
  2. Not sure what kind of tests we should have. It seems they should be quite complicated to be useful. I've added a simple one to start out with.

@joss-aztec
Copy link
Contributor

Not sure what kind of tests we should have. It seems they should be quite complicated to be useful. I've added a simple one to start out with.

Probably out of scope for this PR, but if tested in conjunction with constant folding and DCE, we could:

  1. Take some program with no arguments, foreign functions, or other sources of uncertainty, such that it can be fully solved
  2. Run inlining, constant folding and DCE to check that program reduces to a single instruction returning the expected constant

E.g. check that

add_3 {
  block0(u8 v0):
    v1: add v0 u8 3
    return v1
}

double_and_add_3 {
  block0(u8 v0):
    v1: mul v0 u8 2
    v2: call add_3 v1
    return v2
}

main {
  block0:
    v0: call double_and_add_3 u8 7
    v1: call double_and_add3 v0
    return v1
}

Reduces to

main {
  block0:
    return u8 37
}

I guess we'd also need other simplification passes if were we to test a program containing functions that compromise of more than a single block.

@kevaundray kevaundray added this pull request to the merge queue May 9, 2023
Merged via the queue into master with commit fa1af50 May 9, 2023
@kevaundray kevaundray deleted the jf/ssa-inlining branch May 9, 2023 13:27
TomAFrench added a commit that referenced this pull request May 12, 2023
@TomAFrench
Merge branch 'phated/acvm-0.12.0' into phated/witness-map
aeeceda 
* phated/acvm-0.12.0: (45 commits)
  chore!: Update to ACVM 0.12.0
  official release of backend
  feat: use dummy constructor for bb call
  chore: add missing `?`
  chore: use `try_vecmap` in old `vecmap` locations
  chore: update `acvm-backend-barretenberg` to 0.1.0 commit
  latest master
  fix: improve variable resolution
  test: re enabled sort test
  chore: update cargo tomls
  feat: adapted to heterogeneous bb calls
  remove unneeded import
  fix grep problems
  chore: replace long `Backend` type parameters with `B`
  update to latest commit
  chore: Make CliError generic over a Backend
  chore: Update nargo core to return backend errors
  chore!: Update to acvm 0.11.0
  chore(parser): Parser error optimisation (#1292)
  chore(ssa refactor): Implement function inlining (#1293)
  ...
TomAFrench added a commit that referenced this pull request May 17, 2023
@TomAFrench
Merge branch 'master' into jf/fix-1110
df0d879 
* master: (66 commits)
  feat(nargo)!: retire print-acir in favour of flag (#1328)
  chore(ssa): enable cse for assert (#1350)
  chore(ssa refactor): Add basic instruction simplification (#1329)
  chore(noir): Release 0.6.0 (#1279)
  feat: enable to_radix for any field element (#1343)
  chore(ssa refactor): Simplify inlining pass and fix inlining failure (#1337)
  chore!: Update to acvm 0.11.0 (#1322)
  feat: Add ECDSA secp256k1 builtin test (#1294)
  chore: add support for encoding/decoding inputs from JSON (#1325)
  feat: Issue an error when attempting to use a `return` expression (#1330)
  chore(ssa refactor): Fix inlining bug (#1335)
  fix: to-bits and to-radix for > 128 bits (#1312)
  chore(parser): Parser error optimisation (#1292)
  chore(ssa refactor): Implement function inlining (#1293)
  chore: fix installation link in readme (#1326)
  chore: fix installation link in readme (#1326)
  feat(stdlib): Add keccak (#1249)
  fix: Parsing nested generics (#1319)
  chore(ssa refactor): Document some SSA-gen functions (#1321)
  fix: Assigning to tuple fields (#1318)
  ...
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@joss-aztec joss-aztec joss-aztec left review comments

@kevaundray kevaundray kevaundray approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@jfecher @joss-aztec @kevaundray